[PATCH] rhi: vulkan: Fix unintentional SDR format selection

I needed to read back the swapchain image for my Vulkan RHI-enabled
QtQuick application, but it complained that
VK_FORMAT_A2R10G10B10_UNORM_PACK32 wasn't able to be read back - why?

My initial naive solution is to simply add it to the
swapchainReadbackTextureFormat function - but that wouldn't work as
there is no BGR10A2 RHI texture format and applications would unkowingly
end up with swapped channels. The actual problem came down to how we
were selecting swapchain image formats.

The first thing I changed was the hdrFormatMatchesVkSurfaceFormat
function, because that checked two formats for HDR10:
VK_FORMAT_A2B10G10R10_UNORM_PACK32 and
VK_FORMAT_A2R10G10B10_UNORM_PACK32. Checking the online Vulkan hardware
database, the BGR variant is more well-supported. Picking the RGB
variant is inevitably going to lead into the problem described before -
and should be added back when & if a new RHI texture format is
introduced.

The next thing I changed was the swapchain format selection logic,
specifically the choice for a non-sRGB SDR format. Judging by the
comments in this function and other RHIs like DX12 we *want* the default
color format of VK_FORMAT_B8G8R8A8_UNORM unless otherwise requested.
That isn't what was happening though, on my specific hardware it was
choosing VK_FORMAT_A2R10G10B10_UNORM_PACK32 - why?

It comes down to the isSrgbFormat check in the loop. Again, for the
non-SRGB SDR case the "srgbRequested" variable is always false. And when
a non-SRGB format (like the aforementioned problematic VkFormat) is
checked isSrgbFormat will return false, but I don't think that's what is
intended here. We want that for the sRGB case, but for non-SRGB SDR the
default color format is fine and that lines up with other RHIs
(see QD3D12SwapChain::chooseFormats for an example.) I checked this
inside the loop so the passthrough code is still ran on Wayland, but I
think the new logic is still sensible.

I tested this against the five usual cases I could think of and now the
format selection seems sensible:
* non-sRGB SDR chose VK_FORMAT_B8G8R8A8_UNORM
* sRGB SDR chose VK_FORMAT_R8G8B8A8_SRGB
* extended sRGB Linear chose VK_FORMAT_R16G16B16A16_SFLOAT
* HDR10 chose VK_FORMAT_A2B10G10R10_UNORM_PACK32
* Display P3 chose VK_FORMAT_R16G16B16A16_SFLOAT

Change-Id: Ie79e9fcaa1130311958b485af9b73c59d5d9a335
Reviewed-by: Laszlo Agocs <laszlo.agocs@qt.io>
(cherry picked from commit 2dd1aa3678d541aef15b564b4013728ed5b0387b)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit f4c5e54a8703944ec7ea005ad4de3072b86fd61f)

Gbp-Pq: Name upstream_hdr_vulkan.diff

diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp
index a246c86a1e666e1f3be10165f8f554cfe862f41c..12acbaee1b8910597a2b1b9daaf4473db26095ed 100644 (file)
--- a/src/gui/rhi/qrhivulkan.cpp
+++ b/src/gui/rhi/qrhivulkan.cpp
@@ -8691,7 +8691,7 @@ static inline bool hdrFormatMatchesVkSurfaceFormat(QRhiSwapChain::Format f, cons
         return s.format == VK_FORMAT_R16G16B16A16_SFLOAT
                 && s.colorSpace == VK_COLOR_SPACE_EXTENDED_SRGB_LINEAR_EXT;
     case QRhiSwapChain::HDR10:
-        return (s.format == VK_FORMAT_A2B10G10R10_UNORM_PACK32 || s.format == VK_FORMAT_A2R10G10B10_UNORM_PACK32)
+        return s.format == VK_FORMAT_A2B10G10R10_UNORM_PACK32
                 && s.colorSpace == VK_COLOR_SPACE_HDR10_ST2084_EXT;
     case QRhiSwapChain::HDRExtendedDisplayP3Linear:
         return s.format == VK_FORMAT_R16G16B16A16_SFLOAT
@@ -8810,37 +8810,81 @@ bool QVkSwapChain::ensureSurface()
     quint32 formatCount = 0;
     rhiD->vkGetPhysicalDeviceSurfaceFormatsKHR(rhiD->physDev, surface, &formatCount, nullptr);
     QList<VkSurfaceFormatKHR> formats(formatCount);
-    if (formatCount)
-        rhiD->vkGetPhysicalDeviceSurfaceFormatsKHR(rhiD->physDev, surface, &formatCount, formats.data());
+    rhiD->vkGetPhysicalDeviceSurfaceFormatsKHR(rhiD->physDev, surface, &formatCount,
+                                               formats.data());
 
-    // See if there is a better match than the default BGRA8 format. (but if
-    // not, we will stick to the default)
+    // Initially select the first available format, will only be used as a worst-case fallback
+    colorFormat = formats.constFirst().format;
+    colorSpace = formats.constFirst().colorSpace;
+
+    // See if we can find the preferred SDR format
     const bool srgbRequested = m_flags.testFlag(sRGB);
-    for (int i = 0; i < int(formatCount); ++i) {
-        if (formats[i].format != VK_FORMAT_UNDEFINED) {
-            bool ok = srgbRequested == isSrgbFormat(formats[i].format);
-            if (m_format != SDR)
-                ok &= hdrFormatMatchesVkSurfaceFormat(m_format, formats[i]);
+    bool foundBestFormat = false;
+    if (m_format == SDR) {
+        for (int i = 0; i < int(formatCount); ++i) {
+            bool ok;
+            if (srgbRequested) {
+                ok = defaultSrgbColorFormat == formats[i].format;
+            } else {
+                ok = defaultColorFormat == formats[i].format;
+            }
             if (ok) {
+                foundBestFormat = true;
                 colorFormat = formats[i].format;
                 colorSpace = formats[i].colorSpace;
-#if QT_CONFIG(wayland)
-                // On Wayland, only one color management surface can be created at a time without
-                // triggering a protocol error, and we create one ourselves in some situations.
-                // To avoid this problem, use VK_COLOR_SPACE_PASS_THROUGH_EXT when supported,
-                // so that the driver doesn't create a color management surface as well.
-                const bool hasPassThrough = std::any_of(formats.begin(), formats.end(), [this](const VkSurfaceFormatKHR &fmt) {
-                    return fmt.format == colorFormat && fmt.colorSpace == VK_COLOR_SPACE_PASS_THROUGH_EXT;
-                });
-                if (hasPassThrough) {
-                    colorSpace = VK_COLOR_SPACE_PASS_THROUGH_EXT;
-                }
-#endif
                 break;
             }
         }
     }
 
+    // Otherwise pick one that fits our requirements:
+    if (!foundBestFormat && (m_format != SDR || srgbRequested)) {
+        for (int i = 0; i < int(formatCount); ++i) {
+            bool ok = false;
+            if (m_format != SDR) {
+                ok = hdrFormatMatchesVkSurfaceFormat(m_format, formats[i]);
+            } else if (srgbRequested) {
+                ok = isSrgbFormat(formats[i].format);
+            }
+            if (ok) {
+                colorFormat = formats[i].format;
+                colorSpace = formats[i].colorSpace;
+                break;
+            }
+        }
+    }
+
+    // Although this should be rare, warn when we fail to select a suitable format
+    if (m_format != SDR) {
+        VkSurfaceFormatKHR format{};
+        format.format = colorFormat;
+        format.colorSpace = colorSpace;
+        if (!hdrFormatMatchesVkSurfaceFormat(m_format, format)) {
+            qWarning("Failed to select a suitable VkFormat for HDR, using format %d as fallback",
+                     colorFormat);
+        }
+    } else {
+        if (srgbRequested && !isSrgbFormat(colorFormat)) {
+            qWarning("Failed to select a suitable VkFormat for sRGB, using format %d as fallback",
+                     colorFormat);
+        }
+    }
+
+#if QT_CONFIG(wayland)
+    // On Wayland, only one color management surface can be created at a time without
+    // triggering a protocol error, and we create one ourselves in some situations.
+    // To avoid this problem, use VK_COLOR_SPACE_PASS_THROUGH_EXT when supported,
+    // so that the driver doesn't create a color management surface as well.
+    const bool hasPassThrough =
+            std::any_of(formats.begin(), formats.end(), [this](const VkSurfaceFormatKHR &fmt) {
+                return fmt.format == colorFormat
+                        && fmt.colorSpace == VK_COLOR_SPACE_PASS_THROUGH_EXT;
+            });
+    if (hasPassThrough) {
+        colorSpace = VK_COLOR_SPACE_PASS_THROUGH_EXT;
+    }
+#endif
+
     samples = rhiD->effectiveSampleCountBits(m_sampleCount);
 
     quint32 presModeCount = 0;

diff --git a/src/gui/rhi/qrhivulkan_p.h b/src/gui/rhi/qrhivulkan_p.h
index 59d98c309aa948d205488869f451bc42707ba5ca..17ada4f992b3f4b0ff3c0e31c81f5ba9c8f1bbfb 100644 (file)
--- a/src/gui/rhi/qrhivulkan_p.h
+++ b/src/gui/rhi/qrhivulkan_p.h
@@ -625,7 +625,9 @@ struct QVkSwapChain : public QRhiSwapChain
     int bufferCount = 0;
     VkSurfaceKHR surface = VK_NULL_HANDLE;
     VkSurfaceKHR lastConnectedSurface = VK_NULL_HANDLE;
-    VkFormat colorFormat = VK_FORMAT_B8G8R8A8_UNORM;
+    const VkFormat defaultColorFormat = VK_FORMAT_B8G8R8A8_UNORM;
+    const VkFormat defaultSrgbColorFormat = VK_FORMAT_B8G8R8A8_SRGB;
+    VkFormat colorFormat = VK_FORMAT_UNDEFINED;
     VkColorSpaceKHR colorSpace = VK_COLOR_SPACE_SRGB_NONLINEAR_KHR;
     QVkRenderBuffer *ds = nullptr;
     VkSampleCountFlagBits samples = VK_SAMPLE_COUNT_1_BIT;